fix: sort Recently Used Apps by settled transactions#2012
fix: sort Recently Used Apps by settled transactions#2012keshav0479 wants to merge 2 commits intogetAlby:masterfrom
Conversation
|
Hi, Thanks for the PR. But I still think it's important to know when there was any activity at all, as well as if there was monetary activity. There are also problems with the current solution that simply requesting an invoice (but it not being paid) would count as monetary activity. I think this needs more planning before working on an implementation. |
|
Fair point,it is useful to know when an app was last online/connected, even if it didn't spend anything. To fix the sorting without losing that data, how about we split it into two fields? |
|
I wonder if it is possible already to find these apps by doing a DB query that joins apps on the transaction list? then there's no DB changes needed. (as long as the query is efficient enough) |
|
Oh that's a great point, i hadn't considered using the existing transactions table. |
|
@keshav0479 only settled payments count. I think you can look at the transaction state as the settled constant (in constants.go) |
|
Got it, that makes perfect sense regarding the |
|
@keshav0479 sounds good to me, thanks! |
dc8d2cd to
25c5517
Compare
api/api.go
Outdated
|
|
||
| if orderBy == "" { | ||
| orderBy = "last_used_at" | ||
| var totalCount int64 |
There was a problem hiding this comment.
This is incorrect - counting must be done after filtering.
Also, I would undo your changes and make a new sorting parameter for last_economic_activity or something like that.
There was a problem hiding this comment.
or last_settled_transaction maybe
There was a problem hiding this comment.
Updated! Changed the log field to app_id and added last_settled_transaction as a new sorting option, keeping the original logic intact
25c5517 to
1e5cbd8
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds server-side support and API model field for ordering apps by their most recent settled transaction, updates frontend types/hooks/widget to request and display that ordering, and fixes a structured log field name in an event handler. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Frontend as Frontend (Browser)
participant API as API Server
participant DB as Database
User->>Frontend: Open "Latest Used Apps"
Frontend->>API: GET /api/apps?order_by=last_settled_transaction
API->>DB: Query apps LEFT JOIN transactions (state=SETTLED), SELECT MAX(transactions.settled_at) AS last_transaction_at, GROUP BY apps.id, ORDER BY last_transaction_at IS NULL, last_transaction_at DESC, apps.last_used_at
DB-->>API: Return ordered apps list (includes last_transaction_at)
API->>API: Map dbApp.LastTransactionAt → apiApp.LastSettledAt
API-->>Frontend: Respond with ordered apps (includes lastSettledAt)
Frontend-->>User: Render apps in received order using lastSettledAt
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/api.go (1)
526-531: Count query may return incorrect results with GROUP BY.When
orderBy == "last_settled_transaction", the query has aGROUP BY apps.idclause. Calling.Model(&db.App{}).Count(&totalCount)on this grouped query may not return the correct total count of apps—it could count the number of groups or behave unexpectedly depending on the database driver.Consider computing the count before applying the grouping/aggregation for the
last_settled_transactioncase, or use a separate unmodified query for counting.🔧 Suggested approach
+ // Count before applying aggregation for last_settled_transaction + countQuery := api.db + if filters.Name != "" { + if api.db.Dialector.Name() == "postgres" { + countQuery = countQuery.Where("name ILIKE ?", filters.Name+"%") + } else { + countQuery = countQuery.Where("name LIKE ?", filters.Name+"%") + } + } + // ... apply other filters to countQuery ... + var totalCount int64 - result := query.Model(&db.App{}).Count(&totalCount) + result := countQuery.Model(&db.App{}).Count(&totalCount)Alternatively, you could clone the query before adding the SELECT/JOIN/GROUP BY clauses and use the clone for counting.
🤖 Fix all issues with AI agents
In `@api/api.go`:
- Around line 514-521: The orderBy string already contains DESC and causes a
duplicated "DESC" in query.Order(orderBy + " DESC"), and NULL
last_transaction_at values need to be ordered to the end; when handling the
last_settled_transaction branch (the block that calls query.Select(...
MAX(transactions.created_at) as last_transaction_at) / Joins(...) /
Group("apps.id")), set orderBy to a value that includes NULL-safe ordering (for
example using COALESCE(last_transaction_at, '1970-01-01') DESC,
apps.last_used_at DESC) and then stop appending " DESC" later — change the final
call to use query.Order(orderBy) (or ensure orderBy contains no direction if you
intend to append " DESC"), so you neither produce duplicate DESC nor leave NULL
last_transaction_at entries unordered.
|
@keshav0479 LGTM, nice work 🙌 But can we use |
6ebe358 to
3cacb18
Compare
03fd58d to
47f0597
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/api.go (1)
528-543:⚠️ Potential issue | 🔴 CriticalReject unknown
orderByvalues before building the SQL clause.
orderBystill comes from request input, and Line 543 concatenates it directly intoOrder(...). The new frontend literal union does not protect this endpoint from crafted calls, so please whitelist the supported values and build the full order clause server-side.As per coding guidelines, `{api,http}/**/*.go`: Validate all user input at system boundaries; trust internal service calls.🔒 Safer pattern
+ var orderClause string + if orderBy == "" { orderBy = "last_used_at" } - if orderBy == "last_used_at" { - // when ordering by last used at, apps with last_used_at is NULL should be ordered last - orderBy = "last_used_at IS NULL, " + orderBy - } - - if orderBy == "last_settled_transaction" { + + switch orderBy { + case "last_used_at": + orderClause = "last_used_at IS NULL, last_used_at DESC" + case "created_at": + orderClause = "created_at DESC" + case "last_settled_transaction": query = query.Select("apps.*, MAX(transactions.created_at) as last_transaction_at"). Joins("LEFT JOIN transactions ON transactions.app_id = apps.id AND transactions.state = ?", constants.TRANSACTION_STATE_SETTLED). Group("apps.id") - orderBy = "last_transaction_at IS NULL, last_transaction_at DESC, apps.last_used_at" + orderClause = "last_transaction_at IS NULL, last_transaction_at DESC, apps.last_used_at DESC" + default: + return nil, fmt.Errorf("invalid orderBy: %s", orderBy) } - query = query.Order(orderBy + " DESC") + query = query.Order(orderClause)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/api.go` around lines 528 - 543, Reject/whitelist untrusted orderBy values before using them in the SQL ORDER clause: validate the incoming orderBy variable against an allowed set (e.g., "", "last_used_at", "last_settled_transaction", any other supported keys) and map each allowed key to a fully formed, safe SQL ordering expression (e.g., "last_used_at IS NULL, last_used_at DESC" and for "last_settled_transaction" ensure you build the JOIN/Group and use "last_transaction_at IS NULL, last_transaction_at DESC, apps.last_used_at"); if the value is not in the whitelist, fall back to the default ordering, and only then call query.Order(...). Reference the orderBy variable, the query builder block that sets MAX(transactions.created_at) with constants.TRANSACTION_STATE_SETTLED, and ensure no raw request string is concatenated into Order().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/api.go`:
- Around line 537-540: The current SELECT and GROUP uses
MAX(transactions.created_at) to compute last_transaction_at but should use the
settlement timestamp; change the derived column to MAX(transactions.settled_at)
(the DB column for db.Transaction.SettledAt) in the query.Select and keep the
same JOIN filter using constants.TRANSACTION_STATE_SETTLED, and keep the orderBy
logic ("last_transaction_at IS NULL, last_transaction_at DESC,
apps.last_used_at") so apps are ranked by the actual settled time instead of
request creation time.
In `@frontend/src/components/home/widgets/LatestUsedAppsWidget.tsx`:
- Around line 16-18: The widget is still keyed and filtered by lastUsedAt
despite requesting last_settled_transaction from the API; update
LatestUsedAppsWidget to use the API's lastSettledTransaction field instead:
change the useApps consumer to read appsData?.apps[*].lastSettledTransaction
(not lastUsedAt), update usedApps = apps?.filter(x => x.lastSettledTransaction),
and render the relative-time label from lastSettledTransaction; also make the
same replacement for the other occurrences in this component (around the 35-45
region) so filtering, sorting, and display consistently use
lastSettledTransaction.
---
Outside diff comments:
In `@api/api.go`:
- Around line 528-543: Reject/whitelist untrusted orderBy values before using
them in the SQL ORDER clause: validate the incoming orderBy variable against an
allowed set (e.g., "", "last_used_at", "last_settled_transaction", any other
supported keys) and map each allowed key to a fully formed, safe SQL ordering
expression (e.g., "last_used_at IS NULL, last_used_at DESC" and for
"last_settled_transaction" ensure you build the JOIN/Group and use
"last_transaction_at IS NULL, last_transaction_at DESC, apps.last_used_at"); if
the value is not in the whitelist, fall back to the default ordering, and only
then call query.Order(...). Reference the orderBy variable, the query builder
block that sets MAX(transactions.created_at) with
constants.TRANSACTION_STATE_SETTLED, and ensure no raw request string is
concatenated into Order().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09c37dda-6350-48ce-a943-9fb6df1dba1c
📒 Files selected for processing (4)
api/api.gofrontend/src/components/home/widgets/LatestUsedAppsWidget.tsxfrontend/src/hooks/useApps.tsnip47/event_handler.go
✅ Files skipped from review due to trivial changes (1)
- nip47/event_handler.go
47f0597 to
2c9de78
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/src/components/home/widgets/LatestUsedAppsWidget.tsx (1)
16-18:⚠️ Potential issue | 🟠 MajorUse settled-transaction timestamp consistently in this widget.
Line 18 filters on
lastUsedAt, and Line 45 renders relative time fromlastUsedAt. That keeps this widget tied to connection activity and can hide apps that were correctly ranked by settled transactions.Suggested fix
- const usedApps = apps?.filter((x) => x.lastUsedAt); + const usedApps = apps?.filter((x) => x.lastSettledTransaction); ... - {app.lastUsedAt ? dayjs(app.lastUsedAt).fromNow() : "never"} + {app.lastSettledTransaction + ? dayjs(app.lastSettledTransaction).fromNow() + : "never"}Also applies to: 45-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/home/widgets/LatestUsedAppsWidget.tsx` around lines 16 - 18, The widget is filtering and rendering by connection activity (`lastUsedAt`) but the query sorts by `last_settled_transaction`; update the filtering and display to use the settled-transaction timestamp instead. Specifically, change the filter that defines usedApps (currently using apps?.filter((x) => x.lastUsedAt)) to check the settled timestamp property (e.g., x.lastSettledTransactionAt or the exact field returned for "last_settled_transaction"), and update any rendering logic that shows relative time (currently reading x.lastUsedAt) to read that settled timestamp property with optional chaining. Ensure you reference the same property name returned by useApps so filtering and rendering are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/components/home/widgets/LatestUsedAppsWidget.tsx`:
- Around line 16-18: The widget is filtering and rendering by connection
activity (`lastUsedAt`) but the query sorts by `last_settled_transaction`;
update the filtering and display to use the settled-transaction timestamp
instead. Specifically, change the filter that defines usedApps (currently using
apps?.filter((x) => x.lastUsedAt)) to check the settled timestamp property
(e.g., x.lastSettledTransactionAt or the exact field returned for
"last_settled_transaction"), and update any rendering logic that shows relative
time (currently reading x.lastUsedAt) to read that settled timestamp property
with optional chaining. Ensure you reference the same property name returned by
useApps so filtering and rendering are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a1d308d-ac3c-4bfa-9e58-2d543f9bb743
📒 Files selected for processing (4)
api/api.gofrontend/src/components/home/widgets/LatestUsedAppsWidget.tsxfrontend/src/hooks/useApps.tsnip47/event_handler.go
✅ Files skipped from review due to trivial changes (1)
- nip47/event_handler.go
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/hooks/useApps.ts
- api/api.go
|
Updated! Now using |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/api.go (1)
532-547:⚠️ Potential issue | 🟠 MajorWhitelist
orderByvalues before building the SQLORDER BYclause.Line 547 still concatenates
orderBydirectly into the query. Unsupported/tainted values can break ordering behavior and weaken query safety at an API boundary.🔧 Suggested fix
- if orderBy == "" { - orderBy = "last_used_at" - } - if orderBy == "last_used_at" { - // when ordering by last used at, apps with last_used_at is NULL should be ordered last - orderBy = "last_used_at IS NULL, " + orderBy - } - - if orderBy == "last_settled_transaction" { - query = query.Select("apps.*, MAX(transactions.settled_at) as last_transaction_at"). - Joins("LEFT JOIN transactions ON transactions.app_id = apps.id AND transactions.state = ?", constants.TRANSACTION_STATE_SETTLED). - Group("apps.id") - orderBy = "last_transaction_at IS NULL, last_transaction_at DESC, apps.last_used_at" - } - - query = query.Order(orderBy + " DESC") + switch orderBy { + case "", "last_used_at": + // NULLS last for primary sort key + orderBy = "last_used_at IS NULL, last_used_at DESC" + case "created_at": + orderBy = "created_at DESC" + case "last_settled_transaction": + query = query.Select("apps.*, MAX(transactions.settled_at) as last_transaction_at"). + Joins("LEFT JOIN transactions ON transactions.app_id = apps.id AND transactions.state = ?", constants.TRANSACTION_STATE_SETTLED). + Group("apps.id") + orderBy = "last_transaction_at IS NULL, last_transaction_at DESC, apps.last_used_at IS NULL, apps.last_used_at DESC" + default: + return nil, fmt.Errorf("invalid orderBy: %s", orderBy) + } + + query = query.Order(orderBy)As per coding guidelines
{api,http}/**/*.go: "Validate all user input at system boundaries; trust internal service calls".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/api.go` around lines 532 - 547, The orderBy value used in the SQL ORDER BY clause is currently concatenated directly, which risks SQL injection or invalid queries. To fix it, implement a whitelist of allowed orderBy values before building the SQL clause in the relevant function. Validate the orderBy variable to only accept predefined safe column names like "last_used_at" or "last_settled_transaction" before it is used in query.Order. Reject or default unsupported or tainted values to ensure query safety as demonstrated in the conditional logic surrounding orderBy in the code snippet.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/api.go`:
- Around line 532-547: The orderBy value used in the SQL ORDER BY clause is
currently concatenated directly, which risks SQL injection or invalid queries.
To fix it, implement a whitelist of allowed orderBy values before building the
SQL clause in the relevant function. Validate the orderBy variable to only
accept predefined safe column names like "last_used_at" or
"last_settled_transaction" before it is used in query.Order. Reject or default
unsupported or tainted values to ensure query safety as demonstrated in the
conditional logic surrounding orderBy in the code snippet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8937967c-2252-4f85-8b19-c6c5fe68f6c2
📒 Files selected for processing (6)
api/api.goapi/models.gofrontend/src/components/home/widgets/LatestUsedAppsWidget.tsxfrontend/src/hooks/useApps.tsfrontend/src/types.tsnip47/event_handler.go
✅ Files skipped from review due to trivial changes (1)
- nip47/event_handler.go
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/home/widgets/LatestUsedAppsWidget.tsx
- frontend/src/hooks/useApps.ts
3715604 to
1585dd1
Compare
|
Did you test the PR? It gives |
1585dd1 to
cf374e0
Compare
Hey, sorry about that, i only ran unit tests earlier and missed this runtime path. The issue was |
Description
Fixes #1998
Sorts "Recently Used Apps" by settled transactions instead of
LastUsedAt, so apps that actively move value appear at the top.Changes
LEFT JOINtransactionswherestate = 'SETTLED'and sort byMAX(transactions.created_at).Notes
LastUsedAtstill tracks connection activity (useful for debugging).Summary by CodeRabbit
New Features
Bug Fixes